MDEV-38936 Proactive handling of InnoDB tablespace full condition#4721
MDEV-38936 Proactive handling of InnoDB tablespace full condition#4721FarihaIS wants to merge 1 commit into
Conversation
|
|
grooverdan
added
the
External Contribution
|
Feature request, allow additional use cases (example, tablespace becomes larger than expected):
|
FarihaIS
force-pushed
the
mdev-38936
branch
3 times, most recently
from
64ab2ed to
5bd3d38
Compare
|
@mikegriffin I have just pushed some new changes. Could you please take a look and confirm whether the new implementation addresses the additional use cases you mentioned above? Thank you! |
iMineLink
left a comment
iMineLink
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
I left a few comments on the feature.
Since it's only adding logs and not solving actual bugs related to excessive InnoDB tablespace size (like the recently discovered MDEV-38898), please also wait for @dr-m comments.
Nevertheless, it's fair to say that the feature, when disabled, seems to have a small runtime cost (checking a variable in an ATTRIBUTE_COLD function, new members of fil_space_t, whose footprint may be further reduced by reordering to avoid padding, or eliminated by storing only high 32 bits of threshold + reorder).
FarihaIS
force-pushed
the
mdev-38936
branch
3 times, most recently
from
5a0d8ee to
7c0e2a0
Compare
|
@iMineLink Thank you for the detailed review! I have addressed all your comments and updated the PR description to reflect the latest version of the feature. Please let me know if I have missed anything, thank you. I will wait for @dr-m's review in the meantime. |
iMineLink
left a comment
iMineLink
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review points. I have just a couple more points, then it's good for me.
|
@iMineLink thank you for the suggestions again, I've addressed all the new comments as well! Please let me know if you have any other thoughts while we wait for @dr-m's review. |
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
What is this good for? Do you have an example of already implemented external monitoring that would react when some warning messages appear in the server error log?
Could we have something that would better integrate with event handlers and other existing mechanisms?
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
This is a preliminary review. LGTM. Please keep working with Marko on his review.
FarihaIS
force-pushed
the
mdev-38936
branch
2 times, most recently
from
59cedaa to
1f4e0c0
Compare
|
@dr-m thank you for your feedback. I have addressed the two code changes you requested above now. Please let me know if these changes look okay or if they need further modification. As for the questions you asked above,
These warnings would be helpful for external monitoring tools, for example, AWS RDS, which monitors the error log for operational alerts. This follows the same pattern as existing InnoDB warnings (undo truncation, system tablespace full, etc.).
Could you please help guide me to the kind of integration you're looking for? I'm not entirely sure what the new approach would look like, but I'm happy to make the changes once I have a clearer understanding. |
I think @dr-m is after best practices in log message in general and tooling integration. So perhaps MDEV-27147 JSON Error log to STDERR/STDOUT as an option, and perhaps - https://opentelemetry.io/docs/specs/otel/logs/data-model/#events |
@grooverdan Thanks for the pointers! Since this uses |
|
@dr-m thank you for the detailed feedback! I've addressed/responded to all your comments above - could you please take a look and see if there are any other changes needed? |
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
Sorry for the delay. I think that as part of this, we must pay back some maintenance debt of the fil_space_extend() function.
|
@dr-m thank you for the detailed feedback again! I've addressed all your comments above - could you please take a look and see if there are any other changes needed? |
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
The current base d755574 is 555 commits behind the current target branch, or 66 commits if merges are counted as a single commit. This could explain a few compilation and test failures.
Can you please rebase this to the latest main branch?
This should be OK to push after addressing my review comments.
| const uint warning_pct= fil_system.tablespace_size_warning_pct; | ||
|
|
||
| if (threshold == 0) | ||
| return false; |
There was a problem hiding this comment.
We’re unnecessarily reading warning_pct if threshold==0. Actually, I see that my GCC 16.1.0 is reordering the load of warning_pct after the early return false. We could explicitly do that in the source code, to benefit less aggressively optimizing compilers.
There was a problem hiding this comment.
Good point, moved the reading of warning_pct to after the check for threshold == 0
|
|
||
| /* Reset state if threshold or warning percentage changed */ | ||
| const uint32_t threshold_pages= | ||
| static_cast<uint32_t>(threshold / physical_size()); |
There was a problem hiding this comment.
The constructor-style narrowing cast uint32_t(threshold / physical_size()) would be less of an eye-sore, also for the uint8_t and uint64_t casts in this function.
There was a problem hiding this comment.
Makes sense - changed to constructor-style casting for all 4 occurrences
| if (m_last_warning_threshold != threshold_pages || | ||
| m_last_warning_pct != warning_pct) { |
There was a problem hiding this comment.
The following would lead to shorter code with fewer conditional branches:
if ((m_last_warning_threshold ^ threshold_pages) |
(uint{m_last_warning_pct} ^ warning_pct)) {I was astonished by the size difference this made on the compiler output: from over 900 bytes of x86-64-v3 code to a bit over 630. But, I did not check if there were any changes to function inlining.
There was a problem hiding this comment.
That's incredible, I've changed the if statement to match the XOR one above, thank you!
| /* new_size is at most 2^32 and physical_size() at most 2^16, | ||
| so current_bytes * 100 < 2^55, well within uint64_t range. */ |
There was a problem hiding this comment.
I’d use the C notation in C-style comments: 2^32 is 0x22 (34) to me. Let us write 1<<32 and so on, or 1ULL<<32 if we are pedantic.
There was a problem hiding this comment.
Sounds good, changed the comments to use C-style notation!
| /* Warn on every 1% increase */ | ||
| if (display_pct <= m_last_size_warning_pct) | ||
| return false; |
There was a problem hiding this comment.
The comment would be clearer if it were placed after the if block.
There was a problem hiding this comment.
Agreed, moved to after the if block
| if (!fil_space_extend(this, size)) | ||
| return false; |
There was a problem hiding this comment.
I was about to point out that fil_space_extend() should have been declared static, but I now see that xtrabackup_apply_delta() is invoking that function. This seems to be the simplest solution after all.
| mtr->write<4,mtr_t::FORCED>(*header, FSP_HEADER_OFFSET + FSP_SIZE | ||
| + header->page.frame, size_in_header); |
There was a problem hiding this comment.
Only in the old TAB based InnoDB formatting style we split files before a binary operator such as +. Here, the + should be placed at the end of the first line.
There was a problem hiding this comment.
That's true, thanks for catching that - moved the + to the end of the first line!
| /** Check if tablespace size exceeds warning threshold and emit warning. | ||
| @param new_size New size in pages | ||
| @return true if warning was emitted */ | ||
| bool fil_space_t::check_size_warning(uint32_t new_size) noexcept | ||
| { | ||
| const ulonglong threshold= fil_system.tablespace_size_warning_threshold; |
There was a problem hiding this comment.
This function must be declared and defined with the inline attribute, because there is only a single caller, in a likely code path.
Even better could be to declare and define this function as ATTRIBUTE_COLD and make it take a nonzero fil_system.tablespace_size_warning_threshold as a parameter. The only caller would check it:
if (uint64_t threshold= fil_system.tablespace_size_warning_threshold)
return check_size_warning(new_size, threshold)In this way, the impact on the code cache should be minimized in deployments where this feature is disabled by default.
There was a problem hiding this comment.
Added all the changes you suggested above, thank you
InnoDB write failures occur when tablespace files exceed filesystem size
limits. Current behavior logs errors but continues accepting
transactions, causing repeated failures and potential data integrity
issues.
Add proactive monitoring by emitting warnings when InnoDB tablespaces
approach a configurable size threshold.
Key features:
- Two new system variables:
* innodb_tablespace_size_warning_threshold (default 0, disabled):
Maximum tablespace size in bytes before warnings begin
* innodb_tablespace_size_warning_pct (default 85%): Percentage of
threshold at which to start emitting warnings
- Warning frequency:
* Below warning_pct: No warnings
* At or above warning_pct: Every 1% increase (85%, 86%, 87%, etc.)
- Per-tablespace tracking with automatic reset on TRUNCATE/DROP or
threshold/percentage changes
- Zero overhead when threshold is 0
- Progressive warnings capped at 100%
Implementation adds fil_space_t::extend() which consolidates file
extension, size_in_header update, and size warning checks.
Per-tablespace warning state is tracked in fil_space_t
(m_last_size_warning_pct, m_last_warning_threshold, m_last_warning_pct).
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
7 participants
Description
InnoDB write failures occur when tablespace files exceed filesystem size limits (e.g. 16TB on ext4, 2TB on ext3 - varies by filesystem). Current behavior logs errors but continues accepting transactions, causing repeated failures, user disruption, and potential data integrity issues.
Add proactive monitoring by emitting warnings when InnoDB tablespaces approach a configurable size threshold.
Key features:
innodb_tablespace_size_warning_threshold(default 0, disabled): Maximum tablespace size in bytes before warnings begininnodb_tablespace_size_warning_pct(default 85%): Percentage of threshold at which to start emitting warningswarning_pct: No warningswarning_pct: Every 1% increase (85%, 86%, 87%, etc.)TRUNCATE/DROPor threshold/percentage changesImplementation adds
fil_space_t::extend()which consolidates file extension,size_in_headerupdate, and size warning checks. Per-tablespace warning state is tracked infil_space_t(m_last_size_warning_pct,m_last_warning_threshold,m_last_warning_pct).Release Notes
Added proactive InnoDB tablespace size monitoring to prevent filesystem size limit failures. Two new system variables enable configurable warning thresholds with incremental warning frequency:
innodb_tablespace_size_warning_threshold(default 0, disabled): Maximum size before warningsinnodb_tablespace_size_warning_pct(default 85%): When to start warningsWarning frequency:
How can this PR be tested?
Execute the
innodb.tablespace_size_warningtest in mysql-test-run. This commit adds a test in the innodb suite.The test validates:
TRUNCATE TABLEresets warning stateExpected warning behavior in error log:
Below
innodb_tablespace_size_warning_pct(default 85%): No warningsAt or above
innodb_tablespace_size_warning_pct: Every 1% increaseExample:
[Warning] InnoDB: Tablespace 'test/t1' size 7340032 bytes reached 70% of configured threshold of 10485760 bytesBasing the PR against the correct MariaDB version
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.